Fix for app:enable in case of not valid/not existing enterprise key #36399

pako81 · 2019-11-11T10:19:48Z

Description

This PR fixes the current behaviour of the app:enable occ command in case of not valid/not existing enterprise key.

Related Issue

Fixes occ app:enable should exit with proper error message if enterprise key is invalid #36351

Motivation and Context

Currently, when running the app:enable occ command for enabling an enterprise app and the enterprise key is not valid/not existing, the command does not return any warning/error message. Still, the enterprise app stays disabled. Only when looking into the owncloud.log file one will see an error message about the invalid license key.

How Has This Been Tested?

Manually, by: - disabling an enterprise app, - setting an invalid enteprise key, - trying to re-enable that enterprise app. Now, the $appId cannot be enabled because of invalid enteprise key message is shown.

Implemented for both cases in case groups are/are not given as input to the command.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

CLAassistant · 2019-11-11T10:20:15Z

All committers have signed the CLA.

phil-davis · 2019-11-11T11:20:57Z

https://drone.owncloud.com/owncloud/core/21265/3/6

enabling apps
+ echo 'enabling apps'
+ php occ app:enable files_sharing
An unhandled exception has been thrown:
Error: Class 'OCA\Enterprise_Key\EnterpriseKey' not found in /drone/src/core/Command/App/Enable.php:74

It still needs to work for the community edition, when there is no enterprise_key app.

core/Command/App/Enable.php

pako81 · 2019-11-11T14:47:48Z

@phil-davis @micbar better now?

core/Command/App/Enable.php

phil-davis · 2019-12-02T05:55:32Z

@micbar is this progressing?
(could have an acceptance test added if it is planned for release...)

phil-davis · 2019-12-12T11:24:32Z

@pako81 @micbar I rebased and fixed the code-style fail.
Let's see if the "real" CI passes...

code has been adjusted

suppress PhanUndeclaredClassMethod for \OCA\Enterprise_Key\EnterpriseKey Ignore OCA\Enterprise_Key\EnterpriseKey not found for phpstan

codecov · 2019-12-19T12:12:40Z

Codecov Report

Merging #36399 into master will decrease coverage by <.01%.
The diff coverage is 37.5%.

@@             Coverage Diff              @@
##             master   #36399      +/-   ##
============================================
- Coverage     64.68%   64.68%   -0.01%     
- Complexity    19071    19074       +3     
============================================
  Files          1268     1268              
  Lines         74522    74528       +6     
  Branches       1312     1312              
============================================
+ Hits          48207    48209       +2     
- Misses        25930    25934       +4     
  Partials        385      385

Flag	Coverage Δ	Complexity Δ
#javascript	`54.05% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`65.86% <37.5%> (-0.01%)`	`19074 <1> (+3)`

Impacted Files	Coverage Δ	Complexity Δ
core/register_command.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
core/Command/App/Enable.php	`87.87% <42.85%> (-12.13%)`	`8 <1> (+3)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41b46f7...e0b8d86. Read the comment docs.

phil-davis · 2019-12-19T12:12:50Z

And adjusted the unit test so it knows about the new param to the constructor.
Maybe this time CI will pass?

micbar · 2019-12-19T12:14:22Z

And adjusted the unit test so it knows about the new param to the constructor.
Maybe this time CI will pass?

PHpstan fails because the code of Enterprise Key is not available.

phil-davis · 2019-12-19T12:25:47Z

And adjusted the unit test so it knows about the new param to the constructor.
Maybe this time CI will pass?

PHpstan fails because the code of Enterprise Key is not available.

I "fixed" that by making it ignore that - phpstan CI is passing.

phil-davis · 2019-12-19T12:29:47Z

I added a changelog. I think this is ready for review.

There are existing cliProvisioning/enableApp.feature tests that cover enabling an ordinary app - so that ensures that this code change did not break anything for ordinary apps.

We are not easily able to get ourselves an enterprise app and the enterprise_key app etc during core CI. So this change will need manual testing during the release QA process.

phil-davis · 2020-02-13T09:12:22Z

core/Command/App/Enable.php

@@ -70,6 +76,16 @@ protected function execute(InputInterface $input, OutputInterface $output) {
 			return 1;
 		}

+		if (\OC_App::isEnabled('enterprise_key')) {


The issue we found now is that if you have the enterprise app enabled, but have an invalid enterprise key, and you are just trying to enable a non-enterprise app, then the app cannot be enabled. e.g. enabling the testing app does not work.

If there is some way to check if the app being enabled is an Enterprise app, then this could be fixed by:

if (\OC_App::isEnterpriseApp($appId) && \OC_App::isEnabled('enterprise_key')) {

pako81 self-assigned this Nov 11, 2019

pako81 added the 3 - To Review label Nov 11, 2019

pako81 added this to the development milestone Nov 11, 2019

pako81 mentioned this pull request Nov 11, 2019

occ app:enable should exit with proper error message if enterprise key is invalid #36351

Closed

pako81 requested review from xoxys and micbar November 11, 2019 10:25

pako81 force-pushed the fix-app-enable-enterprise branch from 1246c86 to 078ed73 Compare November 11, 2019 11:03

micbar previously requested changes Nov 11, 2019

View reviewed changes

core/Command/App/Enable.php Show resolved Hide resolved

pako81 force-pushed the fix-app-enable-enterprise branch from 078ed73 to 0d837cd Compare November 11, 2019 14:40

micbar reviewed Nov 11, 2019

View reviewed changes

core/Command/App/Enable.php Outdated Show resolved Hide resolved

pako81 force-pushed the fix-app-enable-enterprise branch from 0d837cd to b3353dd Compare November 11, 2019 14:46

phil-davis reviewed Nov 11, 2019

View reviewed changes

core/Command/App/Enable.php Outdated Show resolved Hide resolved

core/Command/App/Enable.php Outdated Show resolved Hide resolved

karakayasemi reviewed Nov 11, 2019

View reviewed changes

core/Command/App/Enable.php Show resolved Hide resolved

phil-davis reviewed Nov 11, 2019

View reviewed changes

core/Command/App/Enable.php Show resolved Hide resolved

pako81 force-pushed the fix-app-enable-enterprise branch 2 times, most recently from 03a177b to ccef819 Compare November 11, 2019 15:34

phil-davis approved these changes Nov 11, 2019

View reviewed changes

core/Command/App/Enable.php Outdated Show resolved Hide resolved

core/Command/App/Enable.php Outdated Show resolved Hide resolved

phil-davis self-requested a review November 11, 2019 15:40

pako81 force-pushed the fix-app-enable-enterprise branch 2 times, most recently from 5750a1c to 5185111 Compare November 11, 2019 15:51

phil-davis force-pushed the fix-app-enable-enterprise branch from 5185111 to cb5b6b4 Compare December 12, 2019 11:23

phil-davis force-pushed the fix-app-enable-enterprise branch 3 times, most recently from df63431 to e2442a6 Compare December 19, 2019 09:13

phil-davis requested a review from micbar December 19, 2019 09:15

phil-davis requested a review from karakayasemi December 19, 2019 09:15

[email protected] and others added 3 commits December 19, 2019 17:56

Fix for app:enable in case of not valid/not existing enterprise key

9a8bd7d

Ignore EnterpriseKey class not found by code analysis tools

af8c037

suppress PhanUndeclaredClassMethod for \OCA\Enterprise_Key\EnterpriseKey Ignore OCA\Enterprise_Key\EnterpriseKey not found for phpstan

Adjust AppsEnableTest for Enable constructor params

6e703aa

phil-davis force-pushed the fix-app-enable-enterprise branch from e2442a6 to 6e703aa Compare December 19, 2019 12:12

Changelog for 36399 app enable with invalid enterprise key

e0b8d86

phil-davis requested a review from sharidas December 19, 2019 12:59

karakayasemi approved these changes Dec 19, 2019

View reviewed changes

phil-davis approved these changes Dec 19, 2019

View reviewed changes

phil-davis merged commit 2ad40f7 into master Dec 19, 2019

delete-merged-branch bot deleted the fix-app-enable-enterprise branch December 19, 2019 15:05

davitol mentioned this pull request Jan 9, 2020

Tracking test results for ownCloud 10.4.0 #36732

Closed

39 tasks

micbar mentioned this pull request Feb 12, 2020

Revert "Fix for app:enable in case of not valid/not existing enterpri… #36947

Merged

phil-davis reviewed Feb 13, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for app:enable in case of not valid/not existing enterprise key #36399

Fix for app:enable in case of not valid/not existing enterprise key #36399

pako81 commented Nov 11, 2019 •

edited by micbar

Loading

CLAassistant commented Nov 11, 2019 •

edited

Loading

phil-davis commented Nov 11, 2019

pako81 commented Nov 11, 2019 •

edited

Loading

phil-davis commented Dec 2, 2019

phil-davis commented Dec 12, 2019

codecov bot commented Dec 19, 2019 •

edited

Loading

phil-davis commented Dec 19, 2019

micbar commented Dec 19, 2019

phil-davis commented Dec 19, 2019

phil-davis commented Dec 19, 2019

phil-davis Feb 13, 2020

Fix for app:enable in case of not valid/not existing enterprise key #36399

Fix for app:enable in case of not valid/not existing enterprise key #36399

Conversation

pako81 commented Nov 11, 2019 • edited by micbar Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

CLAassistant commented Nov 11, 2019 • edited Loading

phil-davis commented Nov 11, 2019

pako81 commented Nov 11, 2019 • edited Loading

phil-davis commented Dec 2, 2019

phil-davis commented Dec 12, 2019

codecov bot commented Dec 19, 2019 • edited Loading

Codecov Report

phil-davis commented Dec 19, 2019

micbar commented Dec 19, 2019

phil-davis commented Dec 19, 2019

phil-davis commented Dec 19, 2019

phil-davis Feb 13, 2020

Choose a reason for hiding this comment

pako81 commented Nov 11, 2019 •

edited by micbar

Loading

CLAassistant commented Nov 11, 2019 •

edited

Loading

pako81 commented Nov 11, 2019 •

edited

Loading

codecov bot commented Dec 19, 2019 •

edited

Loading